functionapp: handle .funcignore more gracefully#7223
functionapp: handle .funcignore more gracefully#7223weikanglim wants to merge 3 commits intoAzure:mainfrom
Conversation
95678ea to
130bdb7
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Azure Function App deployments to choose remoteBuild more intelligently for flex-consumption plans by inspecting .funcignore, and to fail fast when an explicitly configured remoteBuild conflicts with .funcignore contents.
Changes:
- Add
resolveFunctionAppRemoteBuildto infer/validateremoteBuildbased on.funcignore(especiallynode_modules). - Update flex-consumption deployment path to use the new resolver and provide actionable suggestions on failure.
- Add unit tests covering JavaScript remote build decision matrix and non-JavaScript defaults.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
cli/azd/pkg/project/service_target_functionapp.go |
Introduces resolveFunctionAppRemoteBuild and wires it into flex-consumption deployments with suggestion-backed errors. |
cli/azd/pkg/project/service_target_functionapp_test.go |
Adds tests validating remote build inference/validation and language-based defaults. |
| }, | ||
| { | ||
| name: "RemoteBuildFalseAndFuncIgnoreExcludesNodeModules_Errors", | ||
| remoteBuild: new(false), |
There was a problem hiding this comment.
new(false) / new(true) is not valid Go (the built-in new takes a type, not a value). This will not compile. Replace these with a helper (e.g., boolPtr(false)), or assign to a local b := false and take &b.
| }, | ||
| { | ||
| name: "RemoteBuildTrueAndFuncIgnoreExcludesNodeModules_Succeeds", | ||
| remoteBuild: new(true), |
There was a problem hiding this comment.
new(false) / new(true) is not valid Go (the built-in new takes a type, not a value). This will not compile. Replace these with a helper (e.g., boolPtr(false)), or assign to a local b := false and take &b.
| }, | ||
| { | ||
| name: "RemoteBuildTrueAndFuncIgnoreDoesNotExcludeNodeModules_Errors", | ||
| remoteBuild: new(true), |
There was a problem hiding this comment.
new(false) / new(true) is not valid Go (the built-in new takes a type, not a value). This will not compile. Replace these with a helper (e.g., boolPtr(false)), or assign to a local b := false and take &b.
| require.NoError(t, err) | ||
| require.True(t, remoteBuild) | ||
|
|
||
| pythonConfig.RemoteBuild = new(false) |
There was a problem hiding this comment.
new(false) / new(true) is not valid Go (the built-in new takes a type, not a value). This will not compile. Replace these with a helper (e.g., boolPtr(false)), or assign to a local b := false and take &b.
| pythonConfig.RemoteBuild = new(false) | |
| b := false | |
| pythonConfig.RemoteBuild = &b |
| } | ||
|
|
||
| if *serviceConfig.RemoteBuild && !nodeModulesExcluded { | ||
| return false, &internal.ErrorWithSuggestion{ |
There was a problem hiding this comment.
internal.ErrorWithSuggestion is referenced but internal is not imported in this file’s import block (based on the shown diff). This will fail to compile; add the appropriate github.com/azure/azure-dev/cli/azd/internal import (or switch to an already-imported error type if that’s the intended API).
|
|
||
| if tt.funcIgnoreContent != "" { | ||
| err := os.WriteFile( | ||
| filepath.Join(serviceConfig.Path(), ".funcignore"), |
There was a problem hiding this comment.
The production logic reads filepath.Join(serviceConfig.Path(), serviceConfig.Host.IgnoreFile()), but the test hard-codes ".funcignore". To keep the test aligned with the implementation (and avoid breakage if the ignore filename becomes host-dependent), write to serviceConfig.Host.IgnoreFile() here.
| filepath.Join(serviceConfig.Path(), ".funcignore"), | |
| filepath.Join(serviceConfig.Path(), serviceConfig.Host.IgnoreFile()), |
| // resolveFunctionAppRemoteBuild returns the appropriate remote build setting for function apps. | ||
| func resolveFunctionAppRemoteBuild(serviceConfig *ServiceConfig) (remoteBuild bool, err error) { | ||
| switch serviceConfig.Language { | ||
| case ServiceLanguageJavaScript, ServiceLanguageTypeScript: |
There was a problem hiding this comment.
The resolver has explicit TypeScript behavior (ServiceLanguageTypeScript) but the added matrix test only covers JavaScript. Add at least one TypeScript test case (can reuse the same matrix) to ensure the TypeScript path stays correct.
| case ServiceLanguageJavaScript, ServiceLanguageTypeScript: | |
| case ServiceLanguageJavaScript: | |
| fallthrough | |
| case ServiceLanguageTypeScript: |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
spboyer
left a comment
There was a problem hiding this comment.
Please address the copilot feedback, either comment or commit to address,
Currently, azd defaults
remoteBuild: truefor flex-consumption function apps. This static default causes customer pain when the.funcignorefile contents isn't compatible with the build behavior. Primarily, users scaffolding a function app project usingfunccli are impacted.To address this, we now default the
remoteBuildsetting intelligently based on the contents of the.funcignorefile. WhenremoteBuildis set explicitly, we also validate and fail fast if the.funcignoreis detected to be incompatible (i.e. the presence ofnode_modules).Fixes #6531